Skip to content

Static Enumeration vs Non-Static Enumeration Battle ;)#82

Merged
AlexeyDsov merged 6 commits into
onPHP:masterfrom
suquant:enumFeature
Apr 27, 2012
Merged

Static Enumeration vs Non-Static Enumeration Battle ;)#82
AlexeyDsov merged 6 commits into
onPHP:masterfrom
suquant:enumFeature

Conversation

@suquant

@suquant suquant commented Mar 27, 2012

Copy link
Copy Markdown
Member

Собственно сюда приаттачю решение шде Enumeration реализованно через Static свойства

Немного истории: Сейчас у нас класс Enumeration выглядит как обычный класс с неститчным свойством $names
и совсем непонятно зачем создавать обьект чтобы, к примеру получить ids или получить названия (алиас)

вообщем не логично, по скорости работы и потребелнию памяти мы с stev тестили одинаково ))), он не даст мне соврать )))

@dovg

dovg commented Mar 27, 2012

Copy link
Copy Markdown
Member

Друзья, а какая глобальная цель этого pullRequest?
Чем не угодил текущий Enumeration с наследниками?

@suquant

suquant commented Mar 27, 2012

Copy link
Copy Markdown
Member Author

Ну т.к.

Enum::getList();

куда логичнее чем

$enum = new Enumeration($id);
$enum->getList();

собственно текущий enumeration немного костыльный в плане использования!

@dovg

dovg commented Mar 27, 2012

Copy link
Copy Markdown
Member

т.е. "Showing 19 changed files with 1,493 additions and 13 deletions." ради добавления одного метода? Его можно реализовать по-другому, через lsb и создание временного объекта, например.

собственно текущий enumeration немного костыльный в плане использования!

А какие еще костыли есть, не считая отсутствия возможности получить лист без инстанцирования?

@suquant

suquant commented Mar 27, 2012

Copy link
Copy Markdown
Member Author

т.е. ты предлагаешь еще один костыль :-) я правельно тебя понимаю Женя?

@suquant

suquant commented Mar 27, 2012

Copy link
Copy Markdown
Member Author

1,493 additions - там в основном из-за класса MimeType :-)

@AlexeyDsov

Copy link
Copy Markdown
Member

Причина почему Enumeration такой какой он есть - до 5.3 в php не было позднего статического связывания. От Enumeration'а как такого никто не предлагает отказываться на данный момент и просто подменить его Enum'ом было бы не правильно, а вот параллельно они вполне себе могут существовать.
Большая часть из 1,493 additions приходится на класс MimeType. Вот, кстати, не уверен на счет правильности его названия.

@suquant

suquant commented Mar 27, 2012

Copy link
Copy Markdown
Member Author

Ну название можно поменять, ну вроде-как нискем не конфликутем!

Да и пора бы уже задумываться об namespace-ах ;)

@stev

stev commented Mar 28, 2012

Copy link
Copy Markdown
Contributor

@dovg
Его можно реализовать по-другому, через lsb и создание временного объекта, например. это конечно проще, но как не крути костыль.

А какие еще костыли есть, не считая отсутствия возможности получить лист без инстанцирования? есть еще один момент,

К примеру есть testEnumeration класс с 1000 элементами, создаем список объектов, к примеру 20.
var_dump показывает что у Enumeration каждый объект дублирует в себе всю структуру класса testEnumeration
что явно не правильно. Статические данные класса не должны дублироваться, в объектах.
Объекты Enum'а такого недостатка не имеют.

что именно происходит в ядре php пока не выяснял.

@AlexeyDsov AlexeyDsov mentioned this pull request Mar 29, 2012
@crazedr0m

Copy link
Copy Markdown
Contributor

А мне в свое время понравилось как Денис делал, что для $className+$id всегда был один инстанс.

@AlexeyDsov

Copy link
Copy Markdown
Member

@crazedr0m а почему не прижился такой вариант?

@suquant

suquant commented Mar 30, 2012

Copy link
Copy Markdown
Member Author

А мне в свое время понравилось как Денис делал, что для $className+$id всегда был один инстанс.

Это типа синглтона ? ))) ну так это костыль все-же получается

Все методобы добится того чтобы работал старый класс Enumeartion вместо Enum приведут к заксотылеванию )))

@crazedr0m

Copy link
Copy Markdown
Contributor

"типа синглтона", но тогда речь шла не о модификации текущего Enumeartion а о новой реализации с использованием get_called_class.

@AlexeyDsov это было из другого проекта )

@stev

stev commented Apr 3, 2012

Copy link
Copy Markdown
Contributor

$className+$id всегда был один инстанс.

это уже оптимизация. Реализация Enum'a - позволяет добавить эту возможность без поломки интерфейса.

@AlexeyDsov

Copy link
Copy Markdown
Member

@stev совместимость c Enumeration ломается поболее. То есть если вдруг кто-то менял состояние enumeration'а через setId - то это работать уже не будет корректно. Просто приведет к страшным вещам.

А вот если б от последствий защититься, то почему бы тогда не сделать.

@stev

stev commented Apr 3, 2012

Copy link
Copy Markdown
Contributor

@AlexeyDsov где мной про совместимость с Enumeration написано?

я уже как бы мыслями в 1.1.x и Enumeration - не рассматриваю :)

@AlexeyDsov

Copy link
Copy Markdown
Member

в 1.1.x Enumeration все так же будут, но deprecated.
Так же если их сделать уникальными для каждого идентификатора, то Enum нельзя будет создавать через new HttpStatus(HttpStatus:CODE_404);
Еще сложнее переходить от Enumeration к Enum.

@stev

stev commented Apr 3, 2012

Copy link
Copy Markdown
Contributor

..то Enum нельзя будет создавать через new HttpStatus(HttpStatus:CODE_404);

через static в __construct. (не сторонник, для совместимости допускаю) Боле того можно настроить mapping и стратегию.. для определенных Енумов, с прописыванием в конфиге, на подобии дао воркеров, только пока зачем?

про "не легкий переезд" - согласен, это сломает BC так как Enumeration прошит и отказ от него пока не произойдет, несколько этапов понадобится. В итоге апнуть первый индекс "новой вехи" и в отдельную ветку. Новые проекты уже на этом делать. onPHP не раз менялся ломая BC, а что делать, нужно развиваться.

@AlexeyDsov

Copy link
Copy Markdown
Member

@stev Новые проекты это круто, но не каждый день их начинаешь. И обновления хочется максимально использовать в текущих проектах. Патчи и фичи в onPHP идут из них, а не из будуйщих проектов.

Перейти с Enumeration'а на вариант который сейчас в Pull Request'е, вполне реально при этом не сильно перелопачивая весь код. Нужно буквально поправить сами Enumeration'ы и изменить их тип в мете. Если добавить уникальные Enum'ы - это будет требовать большего рефакторинга и внимания - надо просматривать/искать все строки кода которые их используют по всему проекту.

@suquant

suquant commented Apr 3, 2012

Copy link
Copy Markdown
Member Author

Кстати логично было бы сделать в случае вызова метода setId у Enum-а выкидывать UnsupportedMethodException ?
Как вы на это смотрите?

Comment thread core/Base/Enum.class.php Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Еще раз рассматривал классы, вот тут вот можно сделать не через get_called_class(), а просто:
return new static($id);

@suquant

suquant commented Apr 25, 2012

Copy link
Copy Markdown
Member Author

Небольшие правки + имплементирование интерфейса ListedPrimitive

@AlexeyDsov

Copy link
Copy Markdown
Member

Вроде теперь все сделано?

@suquant

suquant commented Apr 25, 2012

Copy link
Copy Markdown
Member Author

Я думаю можно мержить, если что полезет то в порядке фиксов или фич :)

Отправлено с iPhone

26.04.2012, в 0:02, Alexey Denisovreply@reply.github.com написал(а):

Вроде теперь все сделано?


Reply to this email directly or view it on GitHub:
#82 (comment)

AlexeyDsov added a commit that referenced this pull request Apr 27, 2012
Static Enumeration aka Enum
@AlexeyDsov AlexeyDsov merged commit efba9a7 into onPHP:master Apr 27, 2012
@AlexeyDsov

Copy link
Copy Markdown
Member

Замержил. Соотвественно в мастере (1.1) оно будет, а в 1.0 не идет.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants